feat(operator): introduce v0.1 local web UI - #39
Conversation
glesperance
left a comment
There was a problem hiding this comment.
Bottom line: REQUEST CHANGES
Reviewed at HEAD 6f8d489 against origin/main (merge-base 97cd8a3). Controller gate in a clean worktree: lint PASS, tests 13 failed / 1181 passed (uv run pytest -m "not tmux and not ray") — the branch is red against its own suite, all failures inside the changed surface. Two independent read-only review lanes (correctness, quality-security) both returned REQUEST CHANGES; each finding below was verified against source before inclusion.
Findings
1. blocker: branch fails its own test suite (13 failures, all in the changed surface)
test/operator_tests/test_operator_dev_reload.py (10), test_state_detail.py (2), test_web.py (1).
test_prepare_failure_does_not_publish_runand thetest_malformed_preparation_event_rolls_back_startmatrix expectstart_runto raise preparation failures synchronously; with the new async REQUESTING lifecycle it does not raise. Either the tests encode a contract this PR silently broke, or they were not migrated to the new contract — resolve explicitly, not by deletion.test_close_owns_and_terminates_run_during_preparationexpects an error to surface fromstart_runwhen the operator closes mid-preparation; none does (see finding 2).test_browser_listener_serves_packaged_operator_applicationfails because the vendoredsrc/runtime/operator/web_assets/index.htmldoes not match the test's expectation — the packaged asset is stale relative toweb/operator(see finding 6: nothing in CI regenerates or checks it).
2. major: close() races the async preparation thread
src/runtime/operator/operator.py:1240 installs and starts the preparation thread outside the operator lock, after REQUESTING publication. close() (operator.py:1483) joins handle.preparation_thread only when it is non-None, so it can complete shutdown before start_run launches preparation. The failure path (operator.py:1347-1354) then publishes into a closed operator (rejected at operator.py:2411), skips resource cleanup, and lets start_run return after closure. Synchronize preparation-thread ownership with close(), including a final locked _closed check before starting the thread.
3. major: trace re-materialization contaminates across invocations
src/runtime/operator/operator.py:2891-2912 rebuilds trace.steps and evidence.events from all retained events for the node, while the envelope's invocation_id (operator.py:2915) identifies only the latest trace invocation. A node that invokes an agent more than once produces a latest trace contaminated with earlier invocations' iterations, disagreeing with its own header counts. Filter reconstructed content by the finalized trace's invocation ID and add a two-invocation regression with distinct iteration.recorded steps.
4. major: catalog revision compared without operator epoch
src/runtime/operator/client.py:1587 — _install_catalog_locked discards any catalog with revision < self._catalog.revision without checking operator_instance_id. After an operator restart (revisions restart low), the fresh catalog is dropped during reset (client.py:1541) and later CatalogReplaced updates (client.py:1830), while the callback republishes the retained old-epoch catalog (client.py:1787-1793), which the TUI accepts by revision (src/tui/ui_store.py:2380). Compare revisions only within one epoch; always replace on epoch change.
5. major: gRPC-Web error semantics + unbounded browser hydration
src/runtime/operator/web.py:121: request decoding catches onlyValueError, butParseFromString(web.py:316) raisesgoogle.protobuf.message.DecodeError, which is not aValueErrorsubclass (verified). Malformed protobuf therefore yields an INTERNAL trailer instead of INVALID_ARGUMENT. Additionally, iterator failures after chunked headers are sent (web.py:179) escape to the outer handler, which attempts a second HTTP response (web.py:200) instead of translating inside_send_stream. Add malformed-payload and failing-generator regressions.web/operator/src/api.ts:103:loadBaselinefollows every unique continuation token and appends every summary with no page, item, or byte ceiling — a long-retained or faulty operator consumes unbounded browser memory before baseline publication. Add an explicit hydration budget and fail before partial publication.
6. major: incomplete V2 cutover + frontend gates missing from CI
- The V1
flow_nameselector remains besideworkflow_selector(proto/operator.proto:31,198);server.py:62accepts either andclient.py:1240keeps a legacy-name cache dual-populating requests. Project policy is clean V2-only cutovers — remove the legacy selector/fallback/cache.ARCHITECTURE.md:419,432still documentsListFlowsandStreamRunUpdates. .github/workflows/ci.ymlnever invokes the newweb-proto/web-build/web-test/web-benchMakefile targets, so TypeScript generation, compilation, tests, and vendored-asset sync can all regress while CI stays green (finding 1's staleweb_assetsis the first instance). Add the frontend gates plus a clean-diff check after generation/build.- minor
web.py:43:_RPC_METHODSduplicates the generated service descriptor; a future proto RPC will work over native gRPC but 404 over gRPC-Web. Derive routing from the descriptor or add an exact parity test.
Test gaps
test_web.pylacks oversize-body, path-traversal, and disconnect cases (assert >4 MiB body → error trailer; encoded..→ 404; disconnected update stream unsubscribes).- No deterministic REQUESTING-cancel test: block preparation, cancel while REQUESTING, release, assert CANCELLED with no node execution and handle cleanup.
Downstream (Delta) impact is tracked in Trampoline-AI/trampoline-ai.delta#12 — the renames break Delta's facade imports on its next repin; no action needed in this PR beyond keeping the cutover clean per finding 6.
|
Translation:
|
49f4b00 to
43bf408
Compare
Requested changes addressedThe review was performed at 1. Branch test failures / lifecycle contractThe new asynchronous lifecycle is now explicit: 2.
|
43bf408 to
d8a052e
Compare
d8a052e to
6ce454d
Compare
Rationale
The local operator exposed control and observability only through the Textual TUI. Browser consumers could not observe catalog reloads, and historical runs did not retain the topology required to render the workflow that actually executed. This v0.1 adds a local-first web interface while preserving the operator as the authoritative state owner and keeping current workflow definitions distinct from recorded run state.
Summary
web/operator) and package its built assets with the Python runtime. It uses generated gRPC-Web clients, React Flow, CodeMirror, virtualized lists, Tailwind styling, and component/state/API coverage.uv run ava operator --flows examples --webserves the interface on127.0.0.1:7435by default; non-loopback exposure requires explicit trusted-proxy configuration. The listener has no built-in authentication, so this remains a local-development surface.--log-level) for watcher startup/shutdown and reload attempt, success, unchanged, and failure outcomes.Test Plan
make lintmake web-test— 11 Vitest files / 85 tests passed.make web-build— TypeScript checks and packaged Vite asset build passed.uv run pytest test/cli_test.py test/agent/agent_step_test.py test/agent_rlm_logs_test.py test/operator_tests/test_grpc.py test/operator_tests/test_grpc_client_auth_tls.py test/operator_tests/test_operator.py test/operator_tests/test_operator_dev_reload.py test/operator_tests/test_protocol_contract.py test/operator_tests/test_registry.py test/operator_tests/test_run_updates.py test/operator_tests/test_state_detail.py test/operator_tests/test_web.py test/tui_test.py -qtest_operator_dev_reload.py: 11 failures covering live-source reload, preparation/build failure publication, malformed preparation events, cancellation/running ordering, preparation timeout, and close-during-preparation.test_state_detail.py: 3 failures: initial run status isREQUESTINGinstead of the test's expectedPENDING; a test fixture omits the newly accessedpreparation_thread; and an expected large agent-event detail callback is absent.test_web.py::test_browser_listener_serves_packaged_operator_application: the assertion expects the exact legacy<div id="root"></div>markup, while the built application includes classes on that root element.Reviewer Notes
main; the generated protobuf and TypeScript client files plus compiled browser assets are intentionally checked in.Workflowobjects across process or browser boundaries. Retained run topology is immutable and schema-only; execution configuration and declaration instructions remain in the current catalog.